Conversation
|
@Erosenin2 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
The REST limiter retained every bucket indefinitely and reused a resolver that can accept forwarded user identities. REST limits now use verified JWT subjects (or legacy signed
userIdclaims), otherwise client IP. Storage has a hard 10,000-bucket default bound with LRU eviction, and an idle cleanup timer removes expired buckets even without further traffic.Related Issue
Closes #1269
Changes
requireAuth.ts, retaining signature, algorithm, expiry and claim validation. Existing authentication keeps its original userId precedence and signed-gateway support; the REST limiter never uses forwarded identities.subfor REST keys, retaining compatibility with existing signeduserIdtokens. Invalid, expired, inactive or unverifiable tokens use the IP bucket.maxBuckets, defaulting to 10,000, and refresh LRU order on checks, including denied requests.peekremains non-consuming.resetanddisposerelease timers, and later use restarts cleanup. Validate positive integer limits and supported timer intervals.Security and Compatibility
Rotating
x-user-idcannot obtain fresh REST buckets, including when signed forwarding is enabled for route authentication. JWT subjects are read only after cryptographic verification. Existing IP extraction, fixed-window quotas, 429 body and Retry-After behavior remain unchanged. No dependencies or billing production logic change.Capacity eviction can reset a displaced client's quota under saturation, as with the existing bounded gateway store; the limit is per process and does not provide distributed coordination. Recent active/denied clients retain their buckets preferentially. Cleanup costs at most one scan of the bounded map per window, and timers do not keep the process alive. Removed custom limiters should call
dispose().Verification Results
Upstream currently lacks
package.jsonandjest.env-setup.cjsafter commit 599ab6e. Local validation used copies from that commit's verified parent, excluded from this PR. Installed the existing lockfile withnpm ci --ignore-scripts; no dependencies changed.npm test -- src/middleware/restRateLimit.test.ts src/routes/billing.ratelimit.test.ts --runInBandpretesterror-catalog drift insrc/errors/codes.ts,docs/error-codes.mdanddocs/openapi.json.node node_modules/jest/bin/jest.js src/middleware/restRateLimit.test.ts src/routes/billing.ratelimit.test.ts tests/integration/requireAuth.test.ts --runInBand --forceExitnode node_modules/jest/bin/jest.js src/middleware/restRateLimit.test.ts tests/integration/requireAuth.test.ts --runInBand --forceExit --silentnode node_modules/jest/bin/jest.js src/middleware/restRateLimit.test.ts --runInBand --coverage --collectCoverageFrom=src/middleware/restRateLimit.ts --coverageReporters=text --forceExitnode node_modules/typescript/bin/tsc --noEmitnode node_modules/eslint/bin/eslint.js src/middleware/restRateLimit.ts src/middleware/restRateLimit.test.ts src/middleware/requireAuth.tsajv/lib/refs/json-schema-draft-04.json.git diff --checkWorkspace-wide build/CI cannot be certified while the missing manifest and baseline errors remain. This change does not alter those unrelated files or weaken validation.
x-user-idper request does not reset the limitrestRateLimit.test.tscovers eviction